vm: add dynamic import callback and streamline initialize import.meta callback to match it #19717

devsnek · 2018-04-01T01:43:08Z

Fixes #19570
Fixes #19363

Commits:

src: flatten ContextifyScript
src, vm: implement scoped import.meta and dynamic import
repl: enable dynamic import

This might be a little rough around the edges particularly with the c++ changes, i wasn't quite sure about the difference between the two map types except for that one had the methods i wanted 😓

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

vsemozhetbyt · 2018-04-01T07:17:55Z

doc/api/vm.md

@@ -437,6 +437,9 @@ changes:
    `cachedData` property of the returned `vm.Script` instance.
    The `cachedDataProduced` value will be set to either `true` or `false`
    depending on whether code cache data is produced successfully.
+  * `linker` {function} See [`module.link()`][]


Nit: {function} -> {Function}.

sidenote: maybe we can add a lint rule for this 😓

vsemozhetbyt · 2018-04-01T07:18:30Z

doc/api/vm.md

@@ -891,6 +894,7 @@ associating it with the `sandbox` object is what this document refers to as
 [`script.runInContext()`]: #vm_script_runincontext_contextifiedsandbox_options
 [`script.runInThisContext()`]: #vm_script_runinthiscontext_options
 [`url.origin`]: https://nodejs.org/api/url.html#url_url_origin
+[`module.link()`]: #vm_module_link_linker


Out of ABC-order?

nothing else here follows ABC order so i have no idea where to put it

Sorry, it seems so(
Feel free to ignore.

Oh, I am wrong, there is an ASCII order:

` + Uppercase letter

` + lowercase letter

Uppercase letter

lowercase letter

So this goes after [`eval()`]:

impressive detective work, thank you very much

vsemozhetbyt · 2018-04-01T07:21:12Z

doc/api/vm.md

@@ -437,6 +437,9 @@ changes:
    `cachedData` property of the returned `vm.Script` instance.
    The `cachedDataProduced` value will be set to either `true` or `false`
    depending on whether code cache data is produced successfully.
+  * `linker` {function} See [`module.link()`][]
+    * `specifier` {string}
+    * `scriptOrModule` {Script | Module}


Both Script and Module need to be added in customTypesMap.

If I am not mistaken, in this way, in the end of the object literal, after the url module types:

- 'URLSearchParams': 'url.html#url_class_urlsearchparams' + 'URLSearchParams': 'url.html#url_class_urlsearchparams', + + 'Module': 'vm.html#vm_class_vm_module', + 'Script': 'vm.html#vm_class_vm_script'

guybedford

Will be great to see this in. Just a few questions and comments on naming would be nice to clarify.

guybedford · 2018-04-01T15:02:49Z

lib/internal/errors.js

@@ -763,6 +763,8 @@ E('ERR_HTTP_INVALID_HEADER_VALUE',
 E('ERR_HTTP_INVALID_STATUS_CODE', 'Invalid status code: %s', RangeError);
 E('ERR_HTTP_TRAILER_INVALID',
  'Trailers are invalid with this transfer encoding', Error);
+E('ERR_IMPORT_MODULE_DYNAMICALLY_CALLBACK_MISSING',


Perhaps just ERR_DYNAMIC_IMPORT_CALLBACK_MISSING?

guybedford · 2018-04-01T15:03:17Z

lib/internal/modules/cjs/loader.js

@@ -646,7 +646,11 @@ Module.prototype._compile = function(content, filename) {
  var compiledWrapper = vm.runInThisContext(wrapper, {
    filename: filename,
    lineOffset: 0,
-    displayErrors: true
+    displayErrors: true,
+    importModuleDynamically: experimentalModules ? async (specifier) => {


Again, perhaps just dynamicImport or resolveDynamicImport?

guybedford · 2018-04-01T15:09:25Z

lib/repl.js

@@ -249,7 +250,15 @@ function REPLServer(prompt,
        }
        script = vm.createScript(code, {
          filename: file,
-          displayErrors: true
+          displayErrors: true,
+          async linker(specifier, scriptOrModule) {


This will only be called for top-level dynamic imports right? If so, won't scriptOrModule always be the top-level script above?

It looks like this is exactly the same sort of signature as importModuleDynamically in the vm.runInThisContext case? If so, why not give it the same name?

linkers are always inherited if the child import doesn't have one. as for the signature, importModuleDynamically above was the old name, i forgot to change it, and so it appears i'm missing a test :p

linkers are always inherited if the child import doesn't have one

So if I return a custom VM module instance, that does not have its own dynamic import hook, then this linker will be applied? But it will still only apply to dynamic import?

So if I return a custom VM module instance, that does not have its own dynamic import hook, then this linker will be applied?

yep

But it will still only apply to dynamic import?

vm.Module uses the linker for static and dynamic import requests, the only difference is when the function is called.

Do you mean to say that this same function is run for resolving the static import dependencies of dynamic import modules? But then it should take a Module object as the return value not a Namespace surely?

Also this code should be altered to return module records that aren't yet executed, as it will break circular references running an explicit loader.import here.

Please tell me I'm misinterpreting something here.

vm.Module uses the linker for static and dynamic import requests, the only difference is when the function is called.

Do you mean to say that this same function is run for resolving the static import dependencies of dynamic import modules? But then it should take a Module object as the return value not a Namespace surely?

As far as I can tell neither of the above statements apply to this vm.createScript linker API. Please let me know if I am wrong, but it sees that the linker is acting only as a dynamic import hook, returning the executed namespace object.

As such, perhaps just call it resolveDynamicImport?

i can call it resolveDynamicImport but i would like it to accept objects and vm.Module instances as return values. as to circulars, i'm not sure what you mean. we already handle dynamic import with loader.import(), this doesn't really change that

guybedford · 2018-04-01T20:05:00Z

lib/vm.js

+      const linker = options.linker;
+      const { callLinkerForNamespace } = require('internal/vm/module');
+      options.importModuleDynamically = async (specifier) => {
+        return callLinkerForNamespace(linker, specifier, this);


When linker is undefined, it seems like dynamic import is still throwing here.

It could be worthwhile still defaulting to the default Node loader here.

the only way to get this far is if options.linker is a function, otherwise yes import() rejects with an error about a missing linker. i think defaulting to the process loader would be confusing/unexpected behavior

Ah, thanks for clarifying, of course you've defined the error for this case as well - looks good.

bmeck

looks good except changing to a multimap

bmeck · 2018-04-02T12:38:53Z

src/env.h

+  };
+
+  std::unordered_map<int, loader::ModuleWrap*> id_to_module_wrap_map;
+  std::unordered_map<


can't this have collisions? We should probably ensure that even if there is a collision we can have the duplicates properly stored. Perhaps use a multimap here?

bmeck · 2018-04-02T12:39:27Z

src/env.h

+
+  std::unordered_map<int, loader::ModuleWrap*> id_to_module_wrap_map;
+  std::unordered_map<
+    v8::Global<v8::Module>, loader::ModuleWrap*, ModuleGlobalHash>


same as above, multimap to handle collisions.

bmeck · 2018-04-02T12:45:44Z

src/node_contextify.h

@@ -9,6 +9,8 @@
 namespace node {
 namespace contextify {

+enum SourceType { kScript = 6633, kModule };


why the number?

to stay out of the way of other users of v8's API, fwiw it's "node" on a phone

targos · 2018-04-03T16:12:50Z

@devsnek Heads up: I just landed #19398 and there is a big conflict with this PR. Note that options should be all validated in JS land now.

guybedford

Great to see things progressing here.

guybedford · 2018-04-04T12:51:53Z

lib/internal/modules/esm/translators.js

+          .loaderPromise;
+        return loader.import(specifier, url);
+      },
+    }),


It would be nice not to have to set these up for every module if we can just have a single general function that handles these as we do currently.... is it not possible to maintain the previous fallback mechanism while distinguishing contexts?

fallback method leaves too much at risk imo. if a module wrap is nullptr for some reason I would rather a dynamic import request failed than went to the context loader.

Alternatively since this object is the same for all ES modules, can we define this as a constant and pass the same object each time?

That does require receiving a second argument in importModuleDynamically to get the referrer though.

I guess that won't really help since ModuleWrap picks these off anyway?

What I'm getting at is that any part of the module pipeline where we can reduce the amount of code running, we should, in the name of supporting fast loading of 1000s of modules. While we're setting this up now, it's worth thinking about, as refactoring of these types of interactions gets trickier as the code gets more complex.

It would just be nice to have a more streamlined approach than defining closures per-module, at least so far as there isn't any technical reason we can't.

when I have time I'll set up some benchmarks for this

Performance work will definitely be a great thing to start working on! But note that once you set up these closures, likely no one will ever change this to something simpler. The best time to write simple code is when you first write it, and conceptual simplicity itself is an important goal - performance aside too.

I'm not blocking anything on this, just commenting here.

guybedford · 2018-04-04T12:53:25Z

doc/api/vm.md

@@ -437,6 +437,9 @@ changes:
    `cachedData` property of the returned `vm.Script` instance.
    The `cachedDataProduced` value will be set to either `true` or `false`
    depending on whether code cache data is produced successfully.
+  * `resolveDynamicImport` {Function} See [`module.link()`][]
+    * `specifier` {string}
+    * `scriptOrModule` {Script | Module}


I wonder if the second argument here is necessary for these use cases if it will always be the top-level script? (I know we're going in circles around this somewhat, but helps to clarify!)

i think it's quite likely most people won't assign a handler to each individual script or module instance so they will depend on it being inherited. I can leave a note explaining this behaviour if you think that would help people.

Then perhaps just document it as parentScript as this format of the function is always a script? Alternatively might it make sense to pass this as a this binding?

parentScriptOrModule works I guess, I'll change it in a bit

targos · 2018-04-04T13:02:11Z

Tip: for easier reviews: https://github.com/nodejs/node/pull/19717/files?w=1

jkrems · 2018-04-04T15:37:51Z

Thanks a lot for working on this!

Some high level feedback:

I would generally expect the import hook to be configured per context, not per vm.Script.
The naming ("dynamic") seems awkward since I would assume it doesn't only handle dynamic import but also importing things further down the import graph? Otherwise a simple "allowImport: true" flag seems to be enough..?

devsnek · 2018-04-04T16:45:33Z

@jkrems if i make it per-context we come back to the issue with interacting with node's builtin loader, i don't want to have any indirect method of handing off a dynamic import request to node's loader because of how this presents an escape from the vm in an unexpected way. i'm also not sure what allowImport is, can you explain further?

jkrems · 2018-04-04T19:52:05Z

@devsnek This looks to mostly affect and be tested with vm.runInThisContext. There's - afaik - no expectation of safety there since the global scope can leak all kinds of things:

> cat foo.js
'use strict';

console.log(typeof require('vm').runInThisContext("process.mainModule.require('fs').readFile"));

> ./node foo.js
function

So blocking or redirecting imports seems to not really add anything - other than a confusing and/or polluted module map in that context (does anything track that the same resolved URL gets loaded only once?).

The interesting bit would be both handling and ensuring a consistent module map for other contexts/realms. To be honest: I'm having some problems following what exactly this PR is aiming for which makes it hard to review. Maybe adding some high level notes on the overall idea might be helpful. :)

jkrems · 2018-04-04T19:55:16Z

I'm also not sure what allowImport is, can you explain further?

allowImport was a strawman option for "if we want to reuse the normal module resolution for anything below dynamic import in the module graph anyhow, wouldn't it be easier to just have a boolean option instead of letting people provide a function that only has one meaningful implementation".

devsnek · 2018-04-05T01:50:44Z

@jkrems that seems oddly specific, seemlingly akin to vm.Script having allowCommonJS?

guybedford · 2018-04-12T15:02:30Z

lib/repl.js

+            const referrer = scriptOrModule.url ||
+              getURLFromFilePath(filename === 'repl' ?
+                path.join(process.cwd(), filename) : filename).href;
+            return loader.import(specifier, referrer);


How come this can work without any isNamespace checks then?

it doesn't work, it's wrong and needs a test :D

guybedford · 2018-04-12T15:09:12Z

Yeah, so thinking about this a little further, I would feel much more comfortable not treating the resolveDynamicImport for vm.Script as a full linker function that will link all modules in the graph so far as they are not already linked, and rather assuming it is just a function that will return an already-executed namespace. Then throwing when having a return value that is a vm.Module that isn't already linked. Perhaps even relying on returning module.namespace directly instead of the vm.Module instance.

Supporting a namespace return value seems necessary if we are to be able to delegate the dynamic import to other loaders regardless, so while I know the isNamespace thing sounds like a block, it seems like we will have that case whatever we do as users will likely want to do things like:

x.mjs

new vm.Script({
  resolveDynamicImport (specifier, referrer) {
    // eg reference own contextual import
    return import(specifier);
  }
});

bmeck · 2018-04-12T15:14:15Z

throwing when having a return value that is a vm.Module that isn't already linked

That would prevent cycles.

guybedford · 2018-04-12T15:19:01Z

@bmeck you can't have cycles between vm.Script and vm.Module as vm.Script can't be imported itself.

devsnek · 2018-04-12T15:23:26Z

I'm happy to accept namespaces, we just have to wait a bit more

bmeck · 2018-04-12T15:28:36Z

@guybedford my concern was around it matching constraints and capabilities of vm.Module#link. What advantage do we get by having differences?

guybedford · 2018-04-12T15:35:13Z

@bmeck the only advantage is a separation of concerns, and simpler mental model for script dynamic import linking in not needing to understand the details of vm.Module's linker to use the dynamic import hook of vm.Script.

I can also get behind renaming back to linker or even moduleLinker and going with option A, also saves us a wait on the v8 update, so we can get it merged right away then add the namespace functionality later on when that's ready.

It's only worth waiting if we think it's a nicer API to wait for. Must admit I'm somewhat on the fence, but in such a case perhaps momentum of (A) should win.

devsnek · 2018-04-13T19:29:44Z

we're almost there 😄 #20016

devsnek · 2018-04-15T23:48:59Z

@guybedford this accepts module namespaces now 🎉

TimothyGu · 2018-04-16T01:08:10Z

doc/api/errors.md

@@ -1023,6 +1023,12 @@ made to mark a stream and dependent of itself.
 `http2.connect()` was passed a URL that uses any protocol other than `http:` or
 `https:`.

+<a id="ERR_LINKER_CALLBACK_MISSING"></a>
+### ERR_LINKER_CALLBACK_MISSING


ERR_DYNAMIC_IMPORT_LINKER_MISSING or something similar.

TimothyGu · 2018-04-16T01:08:20Z

doc/api/vm.md

@@ -437,6 +437,9 @@ changes:
    `cachedData` property of the returned `vm.Script` instance.
    The `cachedDataProduced` value will be set to either `true` or `false`
    depending on whether code cache data is produced successfully.
+  * `resolveDynamicImport` {Function} See [`module.link()`][]
+    * `specifier` {string}
+    * `scriptOrModule` {vm.Script | vm.Module}


Remove space around |.

devsnek · 2018-04-16T02:34:58Z

self note: repl needs a test, not quite sure how to yet though

stale

guybedford

Amazing work getting the v8 integration necessary here, I thought that would have taken months!

I've just done an initial review for now, will provide further feedback soon.

guybedford · 2018-04-17T11:44:04Z

lib/internal/modules/esm/translators.js

+  const {
+    initializeImportMetaMap,
+    importModuleDynamicallyMap
+  } = require('internal/process/esm_loader');


Is this an issue with circular references or something? Surely it's already in the graph at this point that it can be hoisted?

guybedford · 2018-04-17T11:44:16Z

lib/internal/modules/esm/translators.js

+  } = require('internal/process/esm_loader');
+  initializeImportMetaMap.set(module, (meta) => { meta.url = url; });
+  importModuleDynamicallyMap.set(module, async (specifier) => {
+    const loader = await require('internal/process/esm_loader')


This should share the same binding of esm_loader as above.

guybedford · 2018-04-17T11:49:02Z

doc/api/vm.md

@@ -437,6 +437,9 @@ changes:
    `cachedData` property of the returned `vm.Script` instance.
    The `cachedDataProduced` value will be set to either `true` or `false`
    depending on whether code cache data is produced successfully.
+  * `resolveDynamicImport` {Function} See [`module.link()`][]
+    * `specifier` {string}
+    * `scriptOrModule` {vm.Script|vm.Module}


We should probably note the return value expected here - vm.Module | ModuleNamespace.

Out of interest, what is the default context for a vm.Module instance? I'm just wondering if a module were to be a thenable to a vm.Module instance if that would enable the sandbox code to break out into the non-sandbox context?

default context is the context it is created in, i'm not sure how one escapes via thenables though

Neither am I, but it's an interesting thought experiment.

So the context is set on vm.Module construction, not on instantiate or link etc? That seems to be what avoids a possible escalation.

default context is the context it is created in

Minor correction: it’s always the top context.

So the context is set on vm.Module construction, not on instantiate or link etc? That seems to be what avoids a possible escalation.

That, and that there seems to be no reason to allow mixing of contexts in a module graph anyway.

guybedford · 2018-04-17T11:49:52Z

lib/internal/vm/module.js

+  return getWrapFromModule(m, scriptOrModule, linker);
+}
+
+async function callLinkerForNamespace(linker, specifier, scriptOrModule) {


What happens if returning a vm.Module instance that has not been linked (and that has dependencies)? Can we have a test for this error case?

this is already tested by testScript

TimothyGu · 2018-04-17T16:03:04Z

I’d still like to take a more in-depth look at this. I would appreciate it if everyone could hold off on landing this for now.

guybedford · 2018-04-17T16:06:32Z

test/parallel/test-vm-dynamic-import.js

+      if (specifier === 'vm://Y') {
+        assert.strictEqual(scriptOrModule, moduleX);
+        return new vm.Module('export default 5', { url: 'vm://Y' });
+      }


@devsnek neither of the above sources have static imports, so I don't think this covers the unlinked test case I described?

it will also inherit the resolveDynamicImport function for that which makes me think we need to change the name again

Surely resolveDynamicImport should never be inherited from Script to Module, only the linker should be inherited from Module to Module?

I think the hang-up we are having is differentiating scripts and modules too much and differentiating dynamic linking and static linking too much. if we just have "linker" and document that it handles all the things I think it should be fine

devsnek · 2018-05-16T17:19:34Z

i'm going to try and reapproach this in a few days, i'll close this for now

devsnek added vm Issues and PRs related to the vm subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Apr 1, 2018

devsnek requested a review from TimothyGu April 1, 2018 01:43

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 1, 2018

devsnek removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 1, 2018

devsnek force-pushed the refactor/module-callbacks branch from 1dc6a0b to 7f3823f Compare April 1, 2018 01:50

devsnek requested a review from bmeck April 1, 2018 04:24

vsemozhetbyt reviewed Apr 1, 2018

View reviewed changes

guybedford reviewed Apr 1, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch 3 times, most recently from 86f4ee9 to cddc07e Compare April 1, 2018 17:04

guybedford reviewed Apr 1, 2018

View reviewed changes

bmeck suggested changes Apr 2, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch 3 times, most recently from 67be43b to 992bbf3 Compare April 2, 2018 16:42

devsnek force-pushed the refactor/module-callbacks branch 2 times, most recently from f68dbc0 to 8761724 Compare April 4, 2018 03:45

devsnek requested a review from jkrems April 4, 2018 03:50

devsnek force-pushed the refactor/module-callbacks branch 2 times, most recently from 54adddc to 10ae336 Compare April 4, 2018 04:21

guybedford reviewed Apr 4, 2018

View reviewed changes

guybedford reviewed Apr 12, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch from 6eb4a61 to 44919ba Compare April 15, 2018 23:47

TimothyGu reviewed Apr 16, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch from 44919ba to c804a2e Compare April 16, 2018 14:56

devsnek requested review from bmeck and guybedford April 16, 2018 19:45

guybedford reviewed Apr 17, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch 2 times, most recently from 702cc14 to db39f07 Compare April 17, 2018 15:10

guybedford reviewed Apr 17, 2018

View reviewed changes

devsnek force-pushed the refactor/module-callbacks branch 2 times, most recently from 6064fd1 to 3c19204 Compare April 22, 2018 22:11

devsnek added 3 commits April 23, 2018 18:53

src: flatten ContextifyScript

74c1c92

src, vm: implement scoped import.meta and dynamic import

3de362e

repl: enable dynamic import

eee5c1f

devsnek force-pushed the refactor/module-callbacks branch from 3c19204 to eee5c1f Compare April 23, 2018 23:53

devsnek closed this May 16, 2018

vm: add dynamic import callback and streamline initialize import.meta callback to match it #19717

vm: add dynamic import callback and streamline initialize import.meta callback to match it #19717

Conversation

devsnek commented Apr 1, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Apr 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Apr 3, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Apr 4, 2018

jkrems commented Apr 4, 2018

devsnek commented Apr 4, 2018 • edited Loading

jkrems commented Apr 4, 2018

jkrems commented Apr 4, 2018

devsnek commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Apr 12, 2018

bmeck commented Apr 12, 2018

guybedford commented Apr 12, 2018

devsnek commented Apr 12, 2018

bmeck commented Apr 12, 2018 • edited Loading

guybedford commented Apr 12, 2018

devsnek commented Apr 13, 2018

devsnek commented Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Apr 16, 2018 • edited Loading

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Apr 1, 2018 •

edited

Loading

vsemozhetbyt Apr 1, 2018 •

edited

Loading

devsnek Apr 1, 2018 •

edited

Loading

devsnek Apr 2, 2018 •

edited

Loading

devsnek Apr 1, 2018 •

edited

Loading

devsnek commented Apr 4, 2018 •

edited

Loading

bmeck commented Apr 12, 2018 •

edited

Loading

devsnek commented Apr 15, 2018 •

edited

Loading

devsnek commented Apr 16, 2018 •

edited

Loading